src: add proper mutexes for accessing FIPS state #42278

addaleax · 2022-03-10T10:57:45Z

The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections.

nodejs-github-bot · 2022-03-10T10:57:49Z

Review requested:

@nodejs/crypto

addaleax · 2022-03-10T10:58:22Z

src/crypto/crypto_util.cc

+  // TODO: This should not be possible to set from worker threads.
+  // CHECK(env->owns_process_state());


Left out because this could be seen as a semver-major change. If we don’t think it’s semver-major, I’m happy to add this as well.

Is it worth doing this for Node.js 18 as a semver-major anyway?

Yeah, sure. It’s just not quite as important and I didn’t want to make this PR one that wouldn’t be backported.

tniessen · 2022-03-10T11:22:24Z

src/crypto/crypto_util.cc

@@ -136,7 +136,13 @@ bool InitCryptoOnce(Isolate* isolate) {
  return true;
 }

+// Protect accesses to FIPS state with a mutex. This should potentially
+// be part of a larger mutex for global OpenSSL state.
+static Mutex fips_mutex;


This makes sense for semantic reasons, but is my understanding correct that this is technically not required since per_process::cli_options_mutex will already guarantee mutually exclusive access whenever fips_mutex is used?

Yes, that is correct as far as the current state of this PR is concerned. 👍

Thanks for clarifying! 😃

tniessen

LGTM with or without fips_mutex.

tniessen · 2022-03-12T15:32:13Z

I have no idea what's going on with arm CI...

nodejs-github-bot · 2022-04-03T01:55:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43273/

nodejs-github-bot · 2022-04-03T06:10:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43277/

nodejs-github-bot · 2022-04-03T10:49:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43285/

nodejs-github-bot · 2022-04-03T12:00:29Z

Landed in 1c69dfe

richardlau · 2022-04-04T15:08:32Z

src/crypto/crypto_util.cc was created as part of a semver-major refactor #35093 so this will need a manual backport to v14.x-staging if it is required there. It would also need a backport for 12.x but since we're not planning any further 12.x releases after tomorrow's release it's probably a bit late to land there.

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections. PR-URL: nodejs#42278 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections. PR-URL: #42278 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections. PR-URL: nodejs#42278 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections. PR-URL: #42278 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections. PR-URL: nodejs/node#42278 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

src: add proper mutexes for accessing FIPS state

095c3e3

The FIPS state handling and OpenSSL initialization code makes accesses to global OpenSSL state without any protection against parallel modifications from multiple threads. This commit adds such protections.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 10, 2022

addaleax commented Mar 10, 2022

View reviewed changes

addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Mar 10, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2022